[diff] _{reported by reviewdog 🐶}

[diff] _{reported by reviewdog 🐶}

[diff] _{reported by reviewdog 🐶}

[diff] _{reported by reviewdog 🐶}

[diff] _{reported by reviewdog 🐶}

[diff] _{reported by reviewdog 🐶}

[diff] _{reported by reviewdog 🐶}

[diff] _{reported by reviewdog 🐶}

[diff] _{reported by reviewdog 🐶}

[diff] _{reported by reviewdog 🐶}

There are some unresolved comments still, like this one

This has changed the behaviour of the other builtin functions that it's below in this switch, no?
Did you try running file_test?
Also, this change should come with some new tests (probably in check/ for compile-time use and in lower/ for run-time use).

This comment just says what the code says

This just says what the code says

All comments should be complete sentences, I won't keep commenting on them all here

Can we validate that the first parameter is a ClassType of Core.String? And we could write a test with an incorrect type that fails?

I will update the signature validation to require the first parameter to be a ClassType of Core.String instead of AnyType. I can also add a test that attempts to declare StringAt with an incorrect first parameter type. Regarding the test, is there an existing pattern I should follow for validating class types in built-in signatures?

The Check method has a TypeId, which has the information you need. The type instruction would be a ClassType, which has a ClassId that is pointing to a Class.

Eval expects the argument to be a StructValue, why are we checking for pointers here?

I was not sure whether the String argument would be passed by value or by reference at this point in lowering. Is there a helper I should use to determine the representation? Instead of checking the LLVM type?

Hm I am not very familiar lower right now, but it should at least be consistent. It's being received as a "value", and classes have a pointer value-representation by default, so maybe this is always a pointer. Running the test should show you what happens, and I would expect it to be consistent.

Is this extending to i32? Isn't it going to whatever the return type was? Maybe drop the comment?

There are some unresolved comments still, like this one

Do we need to print things to test indexing? I think we could just make a string, and then do an index into it and wrap that in //@dump-sem-ir-begin and //@dump-sem-ir-end so that we can see what semir comes out of it?

no Run in tests

Given these bounds checks are only done for constant values, I am wondering why they are happening here instead of inside eval, more closely matching ArrayIndex behaviour? I think then we don't need the IsStringType check in here at all?

The bounds checks happen here because string indexing goes through the IndexWith interface, not primitive array indexing. I thought to do this to catch constant index errors earlier during semantic analysis before constant evaluation.
I noticed this during tests as well that when I was checking bounds that it only threw errors for fully constant expressions "Test"[4], but it failed to throw for variable bindings let s: str = "Test"; let c: char = s[4]; Even though both the string value and index are compile-time constants. By checking here, the implementation can check bounds for both direct literal indexing and variable-bound strings with constant indices.

I fear I was not clear. I am not wondering why it's using the ArrayIndex implementation. I am wondering if the checking can be done in the implementation of StringAt in eval: https://github.com/carbon-language/carbon-lang/pull/6329/files/153ff8269fd4a57553db0ec1a000d98ebb3279a2#diff-89653437c0b9dd4925413361e1a840632f5f4dd03dc63fee83ce2d04a1586bf1R1716 we have all the information we need here to check and return an Error instead of duplicating a lot of the code here.

I attempted to move the bounds checking to eval, but the StringAt builtin is never being invoked during the check phase for non-constant strings.

// --- fail_negative_index.carbon
library "[[@TEST_NAME]]";

fn TestNegativeIndex() {
  let s: str = "Test";
  //@dump-sem-ir-begin
  let c: char = s[-1];
  //@dump-sem-ir-end
}

For example, the SemIR dump from this test case
%str.as.IndexWith.impl.At.call: init %char = call %bound_method.loc6_21.2(%s.ref, %.loc6_19.2)

This call does not have the the [concrete] annotation, implying it is not being evaluated as a constant.

I suppose the issue lies in MakeConstantForCall, more specifically

 // TODO: Some builtin calls might allow some operands to be non-constant.
  if (!has_constant_operands) {
    if (builtin_kind.IsCompTimeOnly(
            eval_context.sem_ir(), eval_context.inst_blocks().Get(call.args_id),
            call.type_id)) {
      CARBON_DIAGNOSTIC(NonConstantCallToCompTimeOnlyFunction, Error,
                        "non-constant call to compile-time-only function");
      CARBON_DIAGNOSTIC(CompTimeOnlyFunctionHere, Note,
                        "compile-time-only function declared here");
      const auto& function = eval_context.functions().Get(
          std::get<SemIR::CalleeFunction>(callee).function_id);
      eval_context.emitter()
          .Build(inst_id, NonConstantCallToCompTimeOnlyFunction)
          .Note(function.latest_decl_id(), CompTimeOnlyFunctionHere)
          .Emit();
    }
    return SemIR::ConstantId::NotConstant;
  }

Since the string variable s in my test is not constant, the call returns NotConstant before ever reaching MakeConstantForBuiltinCall where the StringAt builtin is. I suppose this limitation is known based on the comment in MakeConstantForCall, however would it be worthwhile to modify MakeConstantForCall to allow StringAt (and potentially other builtins) to be invoked even when operands are not constant?

Eval is constant evaluation. Each instruction has a constant value that is either constant (an instruction representing the constant result) or that is "runtime" (aka NotConstant). See ConstantId::is_constant.

If all the operands of (inputs to) the instruction are constant, then the resulting value can also be constant. That's when we execute StringAt in eval, and produce a constant value as a result. And since we have constant inputs, we can do bounds checks there.

If any of the operands of the instruction are runtime, as in the example you wrote here, then we can not evaluate to a constant value during compile (in eval). Instead, we the call instruction goes through to lower, and we generate runtime code for it.

Does that make sense?

Using the following test case

// --- test_string_indexing.carbon
library "[[@TEST_NAME]]";

fn TestStringIndexing() {
  let s: str = "Test";
  //@dump-sem-ir-begin
  letc: char = s[0];
  //@dump-sem-ir-end
}

The semir shows:

%s.ref: ref %str.ee0ae5.1 = name_ref s, %s
%int_0: Core.IntLiteral = int_value 0 [concrete = constants.%int_0]
...
%str.as.IndexWith.impl.At.call: init %char = call %bound_method.loc6_20.2(%.loc6_17, %int_0)

The NameRef %s.ref does not have the [concrete] annotation, and neither does the resulting Call instruction. The index
is marked [concrete], but the call is not being evaluated as constant.

Even trying a more literal case, where the string itself is concrete

// --- fail_literal_negative_index.carbon
library "[[@TEST_NAME]]";

fn TestLiteralNegativeIndex() {
  //@dump-sem-ir-begin
  let c: char = "Test"[-1];
  //@dump-sem-ir-end
}

Generates this semir

%str: %ptr.fb0 = string_literal "Test" [concrete = constants.%str.0a6]
...
%String.val: %str.ee0ae5.1 = struct_value (%str, %int_4) [concrete = constants.%String.val]
...
str.as.IndexWith.impl.At.call: init %char = call %bound_method.loc9_26.2(%String.val, %.loc9_24.2)

The call itself still is not being evaluated as constant. Is there something preventing interface method calls from being constant evaluated?

In your first example:

  let s: str = "Test";

This is a runtime value. a : denotes a runtime binding.

Is there something preventing interface method calls from being constant evaluated?

Here's another example of an interface with builtin methods:

Here is us calling that to generate a compile-time constant value: https://carbon.godbolt.org/z/jnfTcc4ee (the semir gets truncated)

I think this might help:

The [concrete = on the RHS of the textual semir is the constant value that was output from eval. The call instruction is going into eval, and it is not returning a concrete constant value. It looks like it's returning ConstantId::NotConstant, and you'll probably want to run things in a debugger to figure out what exactly is going on.

Heres what I found, note output from lldb

TEST: toolchain/check/testdata/operators/overloaded/string_indexing.carbon (lldb)  p builtin_kind
(Carbon::SemIR::BuiltinFunctionKind) $0 = {
  Carbon::Internal::EnumBase<Carbon::SemIR::BuiltinFunctionKind, Carbon::SemIR::Internal::BuiltinFunctionKindData::RawEnum, const llvm::StringLiteral *> = (value_ = None)
}

The test attempts to evaluate s[0], which would resolve to the current implementation of At within the IndexWith interface. However, when the evaluator reaches eval, we hit this check

if (builtin_kind == SemIR::BuiltinFunctionKind::None) {
      // TODO: Eventually we'll want to treat some kinds of non-builtin
      // functions as producing constants.
      return SemIR::ConstantId::NotConstant;
    }

At is not a builtin, so we get back NotConstant thereby not hitting StringAt all.

Is there a way to make the constant evaluator "see through" the At wrapper to reach the StringAt builtin?

(Apologies for all the questions on this PR I really appreciate your patience!)

I see, right. Note the different way that EqWith is written:

    fn At[self: Self](subscript: T) -> Char {
      return StringAt(self, subscript);
    }

  fn Equal[self: Self](other: UInt(M)) -> bool = "int.eq";

Instead of writing a function that calls a builtin internally, we should make the At function into the builtin. Sorry I overlooked that this whole time, I didn't forsee that issue.